Skip to content
This repository was archived by the owner on Oct 29, 2024. It is now read-only.

Fixing rounding errors when inserting microseconds #407

Closed
wants to merge 5 commits into from
Closed

Fixing rounding errors when inserting microseconds #407

wants to merge 5 commits into from

Conversation

andre-c-andersen
Copy link
Contributor

When calculating nanoseconds since EPOCH, total_seconds() will give a float representation which when multiplied by 1e9 and cast to int will create rounding errors. E.g. inserting 2017-01-17T08:00:12.000001 with a datetime will produce 2017-01-17T08:00:12.000001024Z in InfluxDB. This commit fixes this by never going via floating seconds, and is based on the total_seconds() function itself.

When calculating nanoseconds since EPOCH, `total_seconds()` will give a float representation which when multiplied by 1e9 and cast to `int` will create rounding errors. E.g. inserting `2017-01-17T08:00:12.000001` with a datetime will produce `2017-01-17T08:00:12.000001024Z` in InfluxDB. This commit fixes this by never going via floating seconds, and is based on the `total_seconds()` function itself.
@@ -16,6 +16,12 @@
EPOCH = UTC.localize(datetime.utcfromtimestamp(0))


def _to_nanos(timestamp):
delta = timestamp - EPOCH
nanos = (delta.days * 86400 + delta.seconds) * 10 ** 9 + delta.microseconds * 10 ** 3
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line fails flake8 (89 chars), mind splitting it up? otherwise, this looks like a good change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The `_to_nanos` function had a line which was too long.
@clslgrnc
Copy link
Contributor

Just encountered this rounding error and was about to submit this very patch.
Can this be merged?

In order to ensure that ns stays an int I would also rewrite:

        if precision is None or precision == 'n':
            return ns
        elif precision == 'u':
            return ns / 1e3
        elif precision == 'ms':
            return ns / 1e6
        elif precision == 's':
            return ns / 1e9
        elif precision == 'm':
            return ns / 1e9 / 60
        elif precision == 'h':
            return ns / 1e9 / 3600

to

        if precision is None or precision == 'n':
            return ns
        elif precision == 'u':
            return ns / 10**3
        elif precision == 'ms':
            return ns / 10**6
        elif precision == 's':
            return ns / 10**9
        elif precision == 'm':
            return ns / 10**9 / 60
        elif precision == 'h':
            return ns / 10**9 / 3600

@clslgrnc
Copy link
Contributor

clslgrnc commented Jan 16, 2020

@sebito91 sebito91 self-assigned this Apr 8, 2020
@sebito91
Copy link
Contributor

sebito91 commented Apr 8, 2020

Duplicate of #649 and fixed in #650

@sebito91
Copy link
Contributor

sebito91 commented Apr 8, 2020

Re-addressed in a new PR that merges this content including fixes from @clslgrnc

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants